Fix unused `stream` in `DefaultPluginXmlFactory#write` by Pankraz76 · Pull Request #2303 · apache/maven

Pankraz76 · 2025-05-07T14:35:03Z

Pull #2303: Fix unused stream in DefaultPluginXmlFactory#write

Blindspot in checkstyle:

Variable os is never used checkstyle/checkstyle#17036

tc:

Pankraz76 · 2025-05-07T14:36:37Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java

@@ -93,7 +93,7 @@ public void write(XmlWriterRequest<PluginDescriptor> request) throws XmlWriterEx
                new PluginDescriptorStaxWriter().write(outputStream, content);
            } else {
                try (OutputStream os = Files.newOutputStream(path)) {


ignore or use: Variable 'os' is never used

this looks like a critical bug and easy to prevent: https://checkstyle.sourceforge.io/checks/coding/unusedlocalvariable.html

bug for bug:

You wanna try adding the rule check to the PR ?

its a bug in checkstyle we already have this checker in place:

Variable os is never used checkstyle/checkstyle#17036

@romani

What version of checkstyle is in use?
Please share link to config also

Please share link to config also

config is mystery ATM as it comes from cloud.

Will update, thanks.

Congo is inherited from parent, but there is a way to override it and complement with the rules needed. You need to analyze how check style is configured in the parent.

We have this bug reproducible and tested already in

Issue #17036: Variable os is never used checkstyle/checkstyle#17039

So its nice to have this info, but not mandatory anymore.

Pankraz76 · 2025-05-07T14:48:38Z

[INFO] BUILD SUCCESS

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java

gnodet

Nice catch !

Pankraz76 · 2025-05-07T20:31:41Z

ty, will add tc as well.

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java

Pankraz76 · 2025-05-08T10:26:02Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java

    }

    /**
-     * Simply converts the given content to an XML string.


'simply' might sound dismissive or oversimplify what readers consider a complex task

Pankraz76 · 2025-05-08T10:26:43Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java

    }

    /**
-     * Simply parse the given xml string.


less is more - kiss.

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java

Pankraz76 · 2025-05-08T10:56:41Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java

-        nonNull(request, "request");
-        Path path = request.getPath();
-        URL url = request.getURL();
-        Reader reader = request.getReader();


With this SOC, we get better visibility into the system and can avoid unnecessary complexity in the code. The current implementation is already clean and follows a functional OOP style, but it's not the final version. There's still room to extract dedicated abstraction layers to support more specialized responsibilities.

However, this approach currently mimics fields of a class, which introduces coupling and reduces cohesion. It leans into feature envy and bypasses SOLID design principles and established architectural patterns. We should aim to refactor accordingly.

gnodet

Removed too much.
Please don't refactor too much without care.

Pankraz76 · 2025-05-08T16:19:11Z

Removed too much. Please don't refactor too much without care.

yes

SOC to myself and
need to go right, to go fast by Uncle Bob.

test first this changes already and things get simpler

test cover DefaultModelProcessor#read #2304

gnodet · 2025-05-08T18:31:06Z

Removed too much. Please don't refactor too much without care.

yes

SOC to myself and

need to go right, to go fast by Uncle Bob.

test first this changes already and things get simpler

test cover DefaultModelProcessor#read #2304

Not sure what you mean, but you removed support for URLs for no good reason. Your first commit what correct, now the PRs is not acceptable.

Pankraz76 · 2025-05-09T11:38:53Z

imho, this branch can never happen:
because of early exit check before its to much defensive over here resulting in unreachable code.

Pankraz76 · 2025-05-09T11:41:18Z

and its DRY anyway so it should resolve:

Pankraz76 · 2025-05-09T11:58:59Z

Not sure what you mean, but you removed support for URLs for no good reason. Your first commit what correct, now the PRs is not acceptable.

yes im sorry. lets SOC this merge and check on follow up to ship this fix and then improve test and everything else dedicated. Thanks for cooperation. Im learn a lot here.

Fix redundant code in DefaultPluginXmlFactory:write #2306

Pankraz76 · 2025-05-09T12:15:04Z

why is flaky build sometimes, not working on master too please?

Pankraz76 · 2025-05-09T20:06:40Z

impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultPluginXmlFactoryTest.java

@@ -0,0 +1,255 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one


test only bug and give follow up as low of test and todo:

[MNG-8729] Use correct outputStream destination; request instead of path in DefaultPluginXmlFactory#write #2312

Pankraz76 · 2025-05-09T20:10:38Z

Your first commit what correct

sorry for high pace. Now bug and test with minimal fix ready to merge and follow up to dig deeper.

[MNG-8729] Use correct outputStream destination; request instead of path in DefaultPluginXmlFactory#write #2312

gnodet · 2025-05-11T11:36:08Z

You should not raise multiple PRs for the same thing, it becomes a mess and we have no idea what to review. Keep a single PR for one fix and update it.

Pankraz76 · 2025-05-11T12:49:12Z

Yes, wanted to provide quick fix upfront.

When merging this the other PR testing everything, therefore having bigger scope could be supplemented.

[MNG-8729] Use correct outputStream destination; request instead of path in DefaultPluginXmlFactory#write #2312

Pankraz76 mentioned this pull request May 7, 2025

Pull #2292: Modernize codebase with Java improvements - SOC DefaultModelProcessor#read #2292

Closed

Pankraz76 commented May 7, 2025

View reviewed changes

Pankraz76 mentioned this pull request May 7, 2025

Variable os is never used checkstyle/checkstyle#17036

Closed

Pankraz76 force-pushed the fix-write branch from 19a5828 to c901eed Compare May 7, 2025 14:46

Pankraz76 marked this pull request as ready for review May 7, 2025 14:48

pzygielo reviewed May 7, 2025

View reviewed changes

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java Outdated Show resolved Hide resolved

Pankraz76 changed the title ~~Pull #3003: fix DefaultPluginXmlFactory#write~~ Pull #3003: fix unused stream in DefaultPluginXmlFactory#write May 7, 2025

Pankraz76 force-pushed the fix-write branch from c901eed to 6f533ad Compare May 7, 2025 14:54

Pankraz76 requested a review from pzygielo May 7, 2025 14:55

Pankraz76 force-pushed the fix-write branch from d178474 to fdbfbc7 Compare May 7, 2025 14:56

gnodet approved these changes May 7, 2025

View reviewed changes

gnodet changed the title ~~Pull #3003: fix unused stream in DefaultPluginXmlFactory#write~~ Fix unused stream in DefaultPluginXmlFactory#write May 7, 2025

Pankraz76 commented May 8, 2025

View reviewed changes

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java Show resolved Hide resolved

Pankraz76 force-pushed the fix-write branch from fdbfbc7 to 92beac8 Compare May 8, 2025 10:23

Pankraz76 commented May 8, 2025

View reviewed changes

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java Show resolved Hide resolved

Pankraz76 force-pushed the fix-write branch from f834642 to f9241cf Compare May 8, 2025 10:50

Pankraz76 commented May 8, 2025

View reviewed changes

Pankraz76 requested a review from gnodet May 8, 2025 10:57

Pankraz76 changed the title ~~Fix unused stream in DefaultPluginXmlFactory#write~~ Fix unused stream in DefaultPluginXmlFactory#write May 8, 2025

gnodet requested changes May 8, 2025

View reviewed changes

Pankraz76 changed the title ~~Fix unused stream in DefaultPluginXmlFactory#write~~ Fix dead code DefaultPluginXmlFactory#write May 9, 2025

Pankraz76 changed the title ~~Fix dead code DefaultPluginXmlFactory#write~~ Pull #3003: Fix dead code DefaultPluginXmlFactory#write May 9, 2025

Pankraz76 changed the title ~~Pull #3003: Fix dead code DefaultPluginXmlFactory#write~~ Pull #3003: Fix dead code in DefaultPluginXmlFactory#write May 9, 2025

Pankraz76 changed the title ~~Pull #3003: Fix dead code in DefaultPluginXmlFactory#write~~ Fix dead code in DefaultPluginXmlFactory#write May 9, 2025

Pankraz76 changed the title ~~Fix dead code in DefaultPluginXmlFactory#write~~ Fix unused stream in DefaultPluginXmlFactory#write May 9, 2025

Pankraz76 mentioned this pull request May 9, 2025

Fix redundant code in DefaultPluginXmlFactory:write #2306

Closed

Pankraz76 changed the title ~~Fix unused stream in DefaultPluginXmlFactory#write~~ Fix unused stream os in DefaultPluginXmlFactory#write May 9, 2025

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 9, 2025

Pull apache#2303: Fix unused stream os in DefaultPluginXmlFactory#write

e2939ec

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 9, 2025

Pull apache#2303: Fix unused stream os in DefaultPluginXmlFactory#write

86735dd

Pankraz76 force-pushed the fix-write branch 2 times, most recently from 86735dd to 83cfb4b Compare May 9, 2025 12:00

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 9, 2025

Pull apache#2303: Fix unused stream os in DefaultPluginXmlFactory#write

83cfb4b

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 9, 2025

Pull apache#2303: Fix unused stream os in DefaultPluginXmlFactory#write

e250003

Pankraz76 force-pushed the fix-write branch from 83cfb4b to e250003 Compare May 9, 2025 12:01

Pankraz76 changed the title ~~Fix unused stream os in DefaultPluginXmlFactory#write~~ Fix unused stream in DefaultPluginXmlFactory#write May 9, 2025

Pankraz76 requested a review from gnodet May 9, 2025 12:30

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 9, 2025

Pull apache#2303: Fix unused stream os in DefaultPluginXmlFactory#write

873bd55

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 9, 2025

Pull apache#2303: Fix unused stream os in DefaultPluginXmlFactory#write

49b52ae

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 9, 2025

Pull apache#2303: Fix unused stream os in DefaultPluginXmlFactory#write

a25c980

Pankraz76 force-pushed the fix-write branch from e250003 to c5fa2c3 Compare May 9, 2025 16:29

Pankraz76 mentioned this pull request May 9, 2025

[MNG-8729] Use correct outputStream destination; request instead of path in DefaultPluginXmlFactory#write #2312

Merged

Pankraz76 commented May 9, 2025

View reviewed changes

Fix unused stream in DefaultPluginXmlFactory#write

ef391f6

Pankraz76 force-pushed the fix-write branch from c5fa2c3 to ef391f6 Compare May 9, 2025 20:08

Pankraz76 closed this May 11, 2025

		@@ -0,0 +1,255 @@
		/*
		* Licensed to the Apache Software Foundation (ASF) under one

Conversation

Pankraz76 commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 commented May 7, 2025

Uh oh!

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

Pankraz76 commented May 7, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

Pankraz76 commented May 8, 2025

Uh oh!

gnodet commented May 8, 2025

Uh oh!

Pankraz76 commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pankraz76 commented May 9, 2025

Uh oh!

Pankraz76 commented May 9, 2025

Uh oh!

Pankraz76 commented May 9, 2025

Uh oh!

Pankraz76 May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 commented May 9, 2025

Uh oh!

gnodet commented May 11, 2025

Uh oh!

Pankraz76 commented May 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Pankraz76 commented May 7, 2025 •

edited

Loading

Pankraz76 May 8, 2025 •

edited

Loading

Pankraz76 commented May 9, 2025 •

edited

Loading

Pankraz76 May 9, 2025 •

edited

Loading